Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove offscreen and parallelize transaction building #1769

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

turbocrime
Copy link
Contributor

@turbocrime turbocrime commented Sep 5, 2024

references #1787

this PR removes the 'offscreen' logic and thus all chrome runtime calls from services. parallel builds now call new Worker directly. the package now uses DOM lib in tsconfig.

keys package now more correctly supports consumers attempting to identify asset paths.

'internal message' types are now removed from the types package.

@turbocrime turbocrime requested review from TalDerei and a team September 5, 2024 05:07
Copy link

changeset-bot bot commented Sep 5, 2024

⚠️ No Changeset found

Latest commit: fb2c9d2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@TalDerei TalDerei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@turbocrime the block processor component still needs to be migrated within the offscreen client – what would that generally look like and how significant of a lift would it be?


let active = 0;

const activateOffscreen = async () => {
Copy link
Contributor

@TalDerei TalDerei Sep 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment: for comparison sake, here's the control flow of the previous implementation:

  1. The offscreenClient object was exported, which includes the buildActions method,

  2. Inside optimisticBuild, offscreenClient.buildActions was invoked to initiate the offscreen document lifecycle.

  3. Within buildActions, an offscreen document was created and managed using chrome.offscreen.createDocument.

  4. Collection of promises and request messages was generated and transmitted to offscreen workers via chrome.runtime.sendMessage.

  5. The extension listens for these requests using a chrome event listener chrome.runtime.onMessage.addListener.

  6. This listener triggers an async IIFE (Immediately Invoked Function Expression) that handles the async action-building process.

  7. The spawnActionBuildWorker function was then called, spawning a web worker for the typescript file (wasm-build-action.ts) using the standard Worker API.

  8. Once the worker event listeners are set up, executeWorker was called to execute the actual wasm module.

* Build actions in parallel by launching a worker for each action in the plan.
* @returns An individually-promised list of build results.
*/
export const launchActionWorkers = (
Copy link
Contributor

@TalDerei TalDerei Sep 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment: for external reviewers, this delineates some of the design considerations based on offline conversations.

The service worker is a single-threaded runtime, and objective is to lift the trial decryption to operate in an offscreen context, preventing it from blocking the service worker. The possible approaches to achieve this are:

  1. target standard dom
  2. require offscreen client to be provided

target standard dom

previously, the services package was responsible for managing the offscreen state by relying on chrome's offscreen API. The services package was refactored to abandon the offscreen API entirely, instead using standard DOM (built-in Worker API) to manage workers.

This approach is more general and works outside of chrome extension limitations. If standard DOM is available, the Worker API is used directly. If not, a polyfill simulates the same environment (e.g., using offscreen workers). Services and block processors can directly create and manage workers using a standardized API, and there’s no dependency on chrome’s runtime or messaging.


require offscreen client to be provided (via context, or constructor)

this approach abstracts the worker-launching mechanism so that services don’t manage workers directly. Instead, workers are launched through a generalized interface, which could be either a standard DOM worker to a chrome offscreen worker. The services packages would be agnostic to the specific worker implementation and lifecycle.

This would require moving the offscreen client into the handler context for services and pass as a parameter for the block processor on initialization, so they can use the same interface to launch and manage offscreen.

In comparison to the first approach, this serves as a generalized worker launching interface provided externally to launch workers, where you're calling a generic interface method rather than a worker constructor (standard DOM).


We opted for the first approach, as the difference between the two is minimal in terms of the actual implementation effort.

actionPlanIndex,
};

const actionWorker = new Worker(new URL('./build-action-worker.js', import.meta.url));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment: previously we were spawning the worker in the extension's offscreen handler, and now it's happening directly in the services package.

Based on offline discussion, a generic offscreen utility called OffscreenWorker now manages the lifecycle of the offscreen document. A polyfill was created where new Worker references the global Worker variable. So when the service worker sets globalThis.Worker = OffscreenWorker, all Worker references in the script now point to OffscreenWorker. Each new Worker call creates an instance of OffscreenWorker, which communicates via a dedicated port to an actual Worker running in the offscreen document.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should also benchmark the web worker performance to identify any potential regressions in the proving time, though there shouldn't be any.

@TalDerei
Copy link
Contributor

TalDerei commented Sep 8, 2024

has there been extensive manual testing that the optimistic proving mechanism doesn't somehow break OffscreenWorker which manages the lifecycle of the offscreen document?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants